Repository navigation
feat(pm-dashboard-backup): per-step run ledger in .last-run.json (#1854 rule 2) - #4058
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 56 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea49a1e1ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif [ "$_name" = "db-dump" ]; then | ||
| { [ "$_outcome" != "ok" ] && [ "$_outcome" != "not-applicable" ]; } && failing="$_name" | ||
| elif [ "$rc" -eq 0 ] && [ "$_outcome" != "ok" ]; then |
There was a problem hiding this comment.
When setup fails before ledger_begin credentials (for example, when bws is missing), this branch selects db-dump as the failing step even though its outcome is not-attempted. Unlike the other steps, this check treats an untouched DB-dump step as the failure regardless of the exit code, overriding the existing STEP fallback. Reproducing the missing-bws case produces failing_step: "db-dump", which the laptop-backups watchdog then reports as the cause. Only use incomplete steps to derive a failure on a zero exit, or otherwise preserve the actual setup failure rather than blaming an unattempted dump.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit: the incomplete-step derivation (including the db-dump branch) now only applies on a zero exit. On a failed exit, the failing step is the in-progress step converted to failed, or the existing $STEP fallback. Verified the missing-bws repro directly: the record now shows failing_step "validate-date" with db-dump "not-attempted" (previously "db-dump"). The record suite passes.
| local dump_script="$home/no-such-dump-script.sh" | ||
| if [[ "$dump_mode" == "ok" ]]; then | ||
| dump_script="$home/dump-ok.sh" | ||
| printf '#!/usr/bin/env bash\nmkdir -p "$HOME/backups/db-dumps/$1"\nprintf "{}\\n" > "$HOME/backups/db-dumps/$1/dump.sql"\n' >"$dump_script" |
There was a problem hiding this comment.
In the Nix build sandbox, /usr/bin/env is unavailable, so the newly generated dump scripts cannot execute. The existing laptop-backups-pm-dashboard-backup-record flake check in lib/default.nix copies the sources and runs this suite directly without patching shebangs; consequently, the dumpok scenario fails at the DB-dump step instead of completing, breaking that check. Generate these shebangs from $BASH, as the other executable stubs in this suite already do, including the failing-dump fixture.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit: both generated dump fixtures (ok and failing) now build their shebang from $BASH via printf "#!%s\n" "$BASH", matching the other executable stubs in this suite, so they execute in the Nix build sandbox where /usr/bin/env is unavailable. The record suite passes.
…b-dump for early setup failures; dump fixtures use resolved bash path
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Refs #1854 (task 5072) — implements rule 2 (one failure ledger per run) for
pm-dashboard-backup, the remaining dashboard gap named in the task packet. The design epic stays open for its other jobs/phases.What
nixos/home/pm-dashboard-backup.shnow keeps one ordered run ledger over the material steps —validate-date, credentials, db-dump, local-archive, r2-upload, drive-upload— with outcomesnot-attempted -> in-progress -> ok | failed, plusnot-applicablefor the one named tolerated case (the optional #1514 DB-dump script being absent; a present dump script is required and its failure fails the step).write_recordserializes the ledger as astepsarray in.last-run.jsonand derives the record'sresult/failing_stepfrom it: an in-progress step on a failed exit becomes the failing step, and on any exit the first required step that is notok(db-dump: notokand notnot-applicable) makes the runfailed— so a failed or interrupted step cannot be recorded as complete even on a zero exit code. Existing fields (failing_step,destinations, sha, timestamps) and the expected-set manifest, destination verification, retention, and watchdog contract are unchanged.Tests
On the laptop:
pm-dashboard-backup-record.test.sh— all passed, now 6 scenarios: happy (full ledger, db-dumpnot-applicable), R2 failure (r2-uploadfailed, drivenot-attempted), SIGTERM during credentials (credentialsfailed, later stepsnot-attempted, exit 143), Drive failure, failing dump script (db-dumpfailed, local-archivenot-attempted), working dump script (db-dumpok).pm-dashboard-backup-manifest.test.sh,pm-dashboard-backup.test.sh,pm-dashboard-backup-copy.test.sh,pm-dashboard-backup-date.test.sh,pm-dashboard-backup-prune.test.sh— all passed unchanged.Need help on this PR? Tag
@codesmith-botwith what you need.